-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Provide a way to create and configure AWSGuard #33
Conversation
Use the packages from NPM rather than requiring global installs of typescript, tslint, and mocha.
src/awsGuard.ts
Outdated
* ec2InstanceNoPublicIP: "mandatory", | ||
* elbAccessLoggingEnabled: "mandatory", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered using the policy names themselves instead of the names like this. However, the policy names include hyphens which would necessitate specifying as strings:
const awsGuard = new AwsGuard({
"ec2-instance-no-public-ip": "advisory",
"elb-logging-enabled": "advisory",
});
I discussed with Erin and doing it this way doesn't feel as idiomatic as is typical with TS/JS where properties are camelCased. I'd prefer to keep these camelCase for TS/JS similar to the args passed to Pulumi resources.
When we eventually add a more general (lower-level) way of configuring policy packs (that the service could use to side-load configure policy packs), which would probably use the policy names (with hyphens) as the keys, we could still use this more idiomatic camelCasing scheme as I've proposed here for the TS API, but under the covers map the camelCase names to hyphen names for use with the built-in configuration mechanism.
* }); | ||
* ``` | ||
*/ | ||
export class AwsGuard extends PolicyPack { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally, I had named this AWSGuardPolicyPack
, but then opted for simply AWSGuard
, and finally changed the casing to match how we case Aws
in API signatures in @pulumi/aws
resulting in AwsGuard
.
constructor(args?: AwsGuardArgs); | ||
constructor(name: string, args?: AwsGuardArgs); | ||
constructor(nameOrArgs?: string | AwsGuardArgs, args?: AwsGuardArgs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may look odd, if you're unfamiliar with overloads in TypeScript. The first two signatures are the two available overloads, and the third is the implementation (the latter not being shown in intellisense).
This allows the following possible ways of constructing:
new AwsGuard();
new AwsGuard({ all: "advisory"});
new AwsGuard("my-custom-name", { all: "advisory" });
declare module "./awsGuard" { | ||
interface AwsGuardArgs { | ||
ec2InstanceDetailedMonitoringEnabled?: EnforcementLevel; | ||
ec2InstanceNoPublicIP?: EnforcementLevel; | ||
ec2VolumeInUseCheck?: EnforcementLevel | Ec2VolumeInUseCheckArgs; | ||
elbAccessLoggingEnabled?: EnforcementLevel; | ||
encryptedVolumes?: EnforcementLevel | EncryptedVolumesArgs; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered having separate interfaces for each policy, e.g.:
export interface Ec2InstanceDetailedMonitoringEnabledPolicyPackArgs {
ec2InstanceDetailedMonitoringEnabled?: EnforcementLevel;
}
export interface Ec2InstanceNoPublicIPPolicyPackArgs {
ec2InstanceNoPublicIP?: EnforcementLevel;
}
// etc.
And then defining AwsGuardArgs
as:
export interface AwsGuardArgs extends
Ec2InstanceDetailedMonitoringEnabledPolicyPackArgs,
Ec2InstanceNoPublicIPPolicyPackArgs,
// etc.
{
all?: EnforcementLevel;
}
Or:
export type AwsGuardArgs =
Ec2InstanceDetailedMonitoringEnabledPolicyPackArgs &
Ec2InstanceNoPublicIPPolicyPackArgs &
// etc.
But then it adds a bunch of unnecessary types and forces you to add the definitions where AwsGuardArgs
is declared.
Doing it the way I've done it is consistent with how we apply "mixins" elsewhere, for example: https://github.com/pulumi/pulumi-aws/blob/daa4d0d4ea9ca5c785f2673f2c87c1865473a506/sdk/nodejs/s3/s3Mixins.ts#L215-L246
src/index.ts
Outdated
// If we're running our integration tests, create a new instance of AwsGuard. | ||
if (process.env["PULUMI_AWSGUARD_TESTING"] === "true") { | ||
const awsGuard = new AwsGuard(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been thinking about whether we want to provide a built-in way that news-up the policy pack from within this library (and I saw what Chris did here), and the more I think about it, the more I think we shouldn't do it. Doing it would be the equivalent of having a resource library like @pulumi/eks
that had some way of newing up an eks.Cluster
within the library itself, rather than requiring you to create your own Pulumi program that does it.
So my feeling is, we should just encourage the creation of your own Policy Pack (it'll be really easy to do because I'll be adding a template for pulumi policy new
).
However, given the way we've currently created our integration tests, we just pass the src directory to --policy-pack
when running the preview, so to keep the integration tests working in the simplest way, for now, I've used this temporary approach. But I'd like to follow-up and remove this from the library and fix-up the integration tests to not do it this way. I can open an issue and add a TODO if you agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally want to disagree. Because needing users to create their own, new, Node module that does nothing but instantiate a new AwsGuard()
is kinda lame. (Since I need to get the files on disk, run npm install
, etc.)
But even in a world where you can use a "built-in" or "default" version of the AWS Guard policy pack, you still need to have the source code on disk anyways. So even then we aren't really saving you any trouble.
So I'm inclined to agree with you. And that we should encourage the creation of custom Policy Packs, and just make that easy via pulumi policy new
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nits, but this looks WAY WAY WAY better than the quick hack I put together. 👍
src/awsGuard.ts
Outdated
/** | ||
* Argument bag for configuring AwsGuard policies. | ||
*/ | ||
// Mixins will be applied to this interface in each module that adds a property to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Mixing the /** */
and //
comment styles here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually did this deliberately. The /** */
is for the JSDoc comment for the API, the //
comment below is an "implementation" comment.
ec2InstanceNoPublicIP?: EnforcementLevel; | ||
ec2VolumeInUseCheck?: EnforcementLevel | Ec2VolumeInUseCheckArgs; | ||
elbAccessLoggingEnabled?: EnforcementLevel; | ||
encryptedVolumes?: EnforcementLevel | EncryptedVolumesArgs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the conciseness of how you are declaring the AwsGuardArgs
args here using mix-ins. But it has the problem of being really confusing as more and more policies are added to the policy pack. A value like encryptedVolumes
not not be unique across all args to all policies.... and the alternative of prefixing the arg with the policy name is kinda lame too.
All this to say: I like the pattern, but I am not sure if it is going to work out for AWS Guard given the large number of rules we plan on adding.
Every alternative I can think of is strictly worse, so let's keep this as-is. Just want calling it out in case this does end up causing a lot of friction, that we should be willing to switch to one of the alternative interfaces you considered if it becomes a big problem. (But it could be I'm being paranoid, and so let's try it this way as it makes policies much easier to author.)
src/index.ts
Outdated
// If we're running our integration tests, create a new instance of AwsGuard. | ||
if (process.env["PULUMI_AWSGUARD_TESTING"] === "true") { | ||
const awsGuard = new AwsGuard(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally want to disagree. Because needing users to create their own, new, Node module that does nothing but instantiate a new AwsGuard()
is kinda lame. (Since I need to get the files on disk, run npm install
, etc.)
But even in a world where you can use a "built-in" or "default" version of the AWS Guard policy pack, you still need to have the source code on disk anyways. So even then we aren't really saving you any trouble.
So I'm inclined to agree with you. And that we should encourage the creation of custom Policy Packs, and just make that easy via pulumi policy new
.
src/security.ts
Outdated
const { enforcementLevel, maxDaysUntilExpiration } = getValueOrDefault(args, { | ||
enforcementLevel: defaultEnforcementLevel, | ||
}); | ||
const maxDays = maxDaysUntilExpiration === undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have getValueOrDefault
be smart enough to take this into account? e.g.
const { enforcementLevel, maxDaysUntilExpiration } = getValueOrDefault(args, {
enforcementLevel: defaultEnforcementLevel,
maxDaysUntilExpiration: 14,
});
src/enforcementLevel.ts
Outdated
import { EnforcementLevel } from "@pulumi/policy"; | ||
|
||
/** @internal */ | ||
export const defaultEnforcementLevel: EnforcementLevel = "mandatory"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we talked about making them "advisory" by default"... im cool with either way just want to know why we changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just matched how we were exporting these currently, but happy to make "advisory"
the default. Chris had mentioned that in his PR as well.
This provides a nicer way to create and configure the AWS Guard policies:
The above is equivalent to:
To make all policies mandatory rather than advisory:
To make all policies mandatory, but change a couple to be advisory:
To disable a particular policy:
To disable all policies except ones explicitly enabled:
To specify configuration for policies that have it: